Conversation
WalkthroughThis PR adds bounded external bencode decoding, UTP-aware magnet resolution and peer seeding, adaptive torrent request scheduling, coordinated peer and tracker lifecycle handling, magnet metadata caching, and related preference, chart, warning, and localization updates. ChangesBitTorrent decoding and orchestration
Interface and preferences
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TorrentEngine
participant MagnetCache
participant MagnetResolver
participant Session
participant TorrentLoop
TorrentEngine->>MagnetCache: Request magnet metadata
MagnetCache->>MagnetResolver: Start or join single-flight resolution
MagnetResolver->>Session: Resolve with listen port and UTP socket
Session-->>MagnetResolver: Return torrent bytes and discovered peers
MagnetResolver-->>MagnetCache: Store successful metadata
MagnetCache-->>TorrentEngine: Return cached metadata and peers
TorrentEngine->>TorrentLoop: Add torrent and seed initial peers
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/risuko-bt/src/peer/connection.rs`:
- Around line 141-154: Update dial_with_transport_order and its try_utp closure
so µTP is never attempted when encryption is required; detect the relevant
RequireEncryption setting from SpawnPeer and fall back to connect(spawn)
instead. Do not call connect_utp_plaintext for encryption-required connections,
preserving µTP only for modes that permit plaintext until MSE-over-µTP is
implemented.
In `@src-tauri/risuko-engine/src/engine/torrent.rs`:
- Around line 91-116: Ensure the spawned task always removes the entry from
`state.in_flight` if `resolver()` panics, using an RAII cleanup guard or
equivalent unconditional cleanup around the task body. Update the logic
associated with `state.in_flight.insert`, `resolver().await`, and
`state.in_flight.remove` so the watch channel is closed and future callers do
not hang after a panic.
In `@src/renderer/components/Preference/Advanced.vue`:
- Line 1398: The btMaxOutstandingPerPeer NumberInput currently prevents the
valid 0 auto/default value. Update the NumberInput configuration associated with
btMaxOutstandingPerPeer to use a minimum of 0, ensuring users can restore the
setting to auto and matching backend behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b514b46-3314-433a-8c21-67088d270b15
📒 Files selected for processing (25)
biome.jsonsrc-tauri/risuko-bt/src/bencode.rssrc-tauri/risuko-bt/src/dht.rssrc-tauri/risuko-bt/src/magnet.rssrc-tauri/risuko-bt/src/peer.rssrc-tauri/risuko-bt/src/peer/connection.rssrc-tauri/risuko-bt/src/piece/piece_tracker.rssrc-tauri/risuko-bt/src/session.rssrc-tauri/risuko-bt/src/storage.rssrc-tauri/risuko-bt/src/torrent.rssrc-tauri/risuko-bt/src/tracker/http.rssrc-tauri/risuko-bt/src/tracker/udp.rssrc-tauri/risuko-bt/src/wire/extended.rssrc-tauri/risuko-engine/src/config/defaults.rssrc-tauri/risuko-engine/src/engine/torrent.rssrc-tauri/src/cli/headless.rssrc/renderer/components/Preference/Advanced.vuesrc/renderer/components/Stats/StatsPage.vuesrc/renderer/components/Task/AddTask.vuesrc/shared/locales/en-US/preferences.tssrc/shared/locales/en-US/task.tssrc/shared/locales/zh-CN/preferences.tssrc/shared/locales/zh-CN/task.tssrc/shared/locales/zh-TW/preferences.tssrc/shared/locales/zh-TW/task.ts
There was a problem hiding this comment.
10 issues found across 25 files
Not reviewed (too large): src-tauri/risuko-bt/src/torrent.rs (~2,057 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/risuko-engine/src/engine/torrent.rs">
<violation number="1" location="src-tauri/risuko-engine/src/engine/torrent.rs:382">
P2: Magnet resolution requests with different options can interfere when they target the same info-hash, because cache singleflight is keyed only by info-hash. Consider including resolver-affecting inputs (at least timeout/tracker/encryption context) in the singleflight key, or limiting singleflight to byte caching after a successful resolve.</violation>
</file>
<file name="src-tauri/src/cli/headless.rs">
<violation number="1" location="src-tauri/src/cli/headless.rs:16">
P2: If tracing was already initialized before start_headless_engine() runs, `try_init()` silently fails and the headless-specific logging config (stderr output, diag/risuko_bt debug levels) is discarded with no indication. Consider logging a `tracing::warn!` or `eprintln!` when try_init() returns Err so operators know the headless logging config isn't active.</violation>
</file>
<file name="src-tauri/risuko-bt/src/wire/extended.rs">
<violation number="1" location="src-tauri/risuko-bt/src/wire/extended.rs:171">
P2: Peer `reqq` values above `u32::MAX` currently wrap during decode instead of being rejected, so invalid large handshakes can be interpreted as a very small request window. Using a checked conversion here keeps handshake parsing consistent with the intended positive-cap semantics and avoids accidental pipeline throttling.</violation>
</file>
<file name="src-tauri/risuko-bt/src/tracker/udp.rs">
<violation number="1" location="src-tauri/risuko-bt/src/tracker/udp.rs:37">
P2: A tracker announce can now be sent to multiple resolved endpoints for the same request before the first success aborts the remaining tasks. For Started/Completed/Stopped events this can duplicate state-changing updates; consider racing only the connect phase or using a staggered/first-success strategy that does not issue parallel announces.</violation>
</file>
<file name="src/renderer/components/Stats/StatsPage.vue">
<violation number="1" location="src/renderer/components/Stats/StatsPage.vue:753">
P2: After changing the time range, the speed tooltip can keep showing old bucket data until the cursor moves because hover state is only reset for mode/protocol toggles. Including stats updates in the hover-reset watcher keeps the chart overlay consistent with freshly loaded data.</violation>
</file>
<file name="src-tauri/risuko-bt/src/bencode.rs">
<violation number="1" location="src-tauri/risuko-bt/src/bencode.rs:336">
P2: Untrusted bencode dictionaries can now include duplicate keys and still parse successfully, leaving downstream code to resolve ambiguous fields by access pattern. Consider relaxing key ordering for external payloads while still rejecting duplicate keys here.</violation>
</file>
<file name="src-tauri/risuko-bt/src/peer/connection.rs">
<violation number="1" location="src-tauri/risuko-bt/src/peer/connection.rs:202">
P2: Identity-failure suppression is brittle because it depends on one exact error string for info-hash mismatch. Using an exact match here misses existing variants (for example MSE mismatch messages), so the code can perform avoidable fallback dials and extra reconnect delay.</violation>
<violation number="2" location="src-tauri/risuko-bt/src/peer/connection.rs:1278">
P2: The peer actor now tears down the socket without an explicit `shutdown()`, so a local disconnect or writer exit may not be observed promptly by the remote peer and any queued bytes rely on task drop instead of an orderly close.</violation>
</file>
<file name="src-tauri/risuko-bt/src/magnet.rs">
<violation number="1" location="src-tauri/risuko-bt/src/magnet.rs:159">
P2: Magnet trackers with a literal comma in their URL can be corrupted before announce, because decoded `tr=` values are passed through `expand_tracker_entries` and split on `,`. Consider only applying comma/newline expansion to caller-supplied tracker lists, while preserving each magnet `tr` parameter as one URL.</violation>
<violation number="2" location="src-tauri/risuko-bt/src/magnet.rs:457">
P2: Peer discovery can drop legitimate peers in non-Cloudflare parts of these address blocks, because several CIDR checks only compare the first two octets despite narrower ranges. Narrow the checks to the actual prefix before using this as a global dial filter.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }; | ||
|
|
||
| let resolved = match info_hash_key { | ||
| Some(key) => self.magnet_cache.get_or_resolve(key, resolve).await?, |
There was a problem hiding this comment.
P2: Magnet resolution requests with different options can interfere when they target the same info-hash, because cache singleflight is keyed only by info-hash. Consider including resolver-affecting inputs (at least timeout/tracker/encryption context) in the singleflight key, or limiting singleflight to byte caching after a successful resolve.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/risuko-engine/src/engine/torrent.rs, line 382:
<comment>Magnet resolution requests with different options can interfere when they target the same info-hash, because cache singleflight is keyed only by info-hash. Consider including resolver-affecting inputs (at least timeout/tracker/encryption context) in the singleflight key, or limiting singleflight to byte caching after a successful resolve.</comment>
<file context>
@@ -234,51 +339,106 @@ impl TorrentEngine {
+ };
+
+ let resolved = match info_hash_key {
+ Some(key) => self.magnet_cache.get_or_resolve(key, resolve).await?,
+ None => Arc::new(resolve().await?),
+ };
</file context>
| use tracing_subscriber::EnvFilter; | ||
| let filter = EnvFilter::try_from_default_env() | ||
| .unwrap_or_else(|_| EnvFilter::new("info,diag=debug,risuko_bt=debug,risuko_engine=info")); | ||
| let _ = tracing_subscriber::fmt() |
There was a problem hiding this comment.
P2: If tracing was already initialized before start_headless_engine() runs, try_init() silently fails and the headless-specific logging config (stderr output, diag/risuko_bt debug levels) is discarded with no indication. Consider logging a tracing::warn! or eprintln! when try_init() returns Err so operators know the headless logging config isn't active.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/src/cli/headless.rs, line 16:
<comment>If tracing was already initialized before start_headless_engine() runs, `try_init()` silently fails and the headless-specific logging config (stderr output, diag/risuko_bt debug levels) is discarded with no indication. Consider logging a `tracing::warn!` or `eprintln!` when try_init() returns Err so operators know the headless logging config isn't active.</comment>
<file context>
@@ -9,11 +9,24 @@ use risuko_engine::engine::manager::TaskManager;
+ use tracing_subscriber::EnvFilter;
+ let filter = EnvFilter::try_from_default_env()
+ .unwrap_or_else(|_| EnvFilter::new("info,diag=debug,risuko_bt=debug,risuko_engine=info"));
+ let _ = tracing_subscriber::fmt()
+ .with_env_filter(filter)
+ .with_writer(std::io::stderr)
</file context>
| .iter() | ||
| .find(|(k, _)| k == b"reqq") | ||
| .and_then(|(_, v)| v.as_int()) | ||
| .and_then(|n| if n > 0 { Some(n as u32) } else { None }); |
There was a problem hiding this comment.
P2: Peer reqq values above u32::MAX currently wrap during decode instead of being rejected, so invalid large handshakes can be interpreted as a very small request window. Using a checked conversion here keeps handshake parsing consistent with the intended positive-cap semantics and avoids accidental pipeline throttling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/risuko-bt/src/wire/extended.rs, line 171:
<comment>Peer `reqq` values above `u32::MAX` currently wrap during decode instead of being rejected, so invalid large handshakes can be interpreted as a very small request window. Using a checked conversion here keeps handshake parsing consistent with the intended positive-cap semantics and avoids accidental pipeline throttling.</comment>
<file context>
@@ -166,11 +164,17 @@ impl ExtHandshake {
+ .iter()
+ .find(|(k, _)| k == b"reqq")
+ .and_then(|(_, v)| v.as_int())
+ .and_then(|n| if n > 0 { Some(n as u32) } else { None });
Some(Self {
supported,
</file context>
| .and_then(|n| if n > 0 { Some(n as u32) } else { None }); | |
| .and_then(|n| u32::try_from(n).ok().filter(|&n| n > 0)); |
| let mut attempts = JoinSet::new(); | ||
| for target in targets { | ||
| let req = req.clone(); | ||
| attempts.spawn(async move { announce_endpoint(target, &req).await }); |
There was a problem hiding this comment.
P2: A tracker announce can now be sent to multiple resolved endpoints for the same request before the first success aborts the remaining tasks. For Started/Completed/Stopped events this can duplicate state-changing updates; consider racing only the connect phase or using a staggered/first-success strategy that does not issue parallel announces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/risuko-bt/src/tracker/udp.rs, line 37:
<comment>A tracker announce can now be sent to multiple resolved endpoints for the same request before the first success aborts the remaining tasks. For Started/Completed/Stopped events this can duplicate state-changing updates; consider racing only the connect phase or using a staggered/first-success strategy that does not issue parallel announces.</comment>
<file context>
@@ -24,11 +26,49 @@ const ACTION_ERROR: u32 = 3;
+ let mut attempts = JoinSet::new();
+ for target in targets {
+ let req = req.clone();
+ attempts.spawn(async move { announce_endpoint(target, &req).await });
+ }
+
</file context>
| if let Some((prev, _)) = items.last() { | ||
| if prev.as_slice() >= key.as_slice() { | ||
| return Err(Error::BadDictOrder(key_pos)); | ||
| if self.dict_mode == DictMode::Canonical { |
There was a problem hiding this comment.
P2: Untrusted bencode dictionaries can now include duplicate keys and still parse successfully, leaving downstream code to resolve ambiguous fields by access pattern. Consider relaxing key ordering for external payloads while still rejecting duplicate keys here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/risuko-bt/src/bencode.rs, line 336:
<comment>Untrusted bencode dictionaries can now include duplicate keys and still parse successfully, leaving downstream code to resolve ambiguous fields by access pattern. Consider relaxing key ordering for external payloads while still rejecting duplicate keys here.</comment>
<file context>
@@ -234,34 +311,36 @@ impl<'a> Parser<'a> {
- if let Some((prev, _)) = items.last() {
- if prev.as_slice() >= key.as_slice() {
- return Err(Error::BadDictOrder(key_pos));
+ if self.dict_mode == DictMode::Canonical {
+ if let Some((prev, _)) = items.last() {
+ if prev.as_slice() >= key.as_slice() {
</file context>
|
|
||
| fn alternate_dial_cannot_help(err: &std::io::Error) -> bool { | ||
| let message = err.to_string(); | ||
| message == "info hash mismatch" || message.starts_with("self-connection ") |
There was a problem hiding this comment.
P2: Identity-failure suppression is brittle because it depends on one exact error string for info-hash mismatch. Using an exact match here misses existing variants (for example MSE mismatch messages), so the code can perform avoidable fallback dials and extra reconnect delay.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/risuko-bt/src/peer/connection.rs, line 202:
<comment>Identity-failure suppression is brittle because it depends on one exact error string for info-hash mismatch. Using an exact match here misses existing variants (for example MSE mismatch messages), so the code can perform avoidable fallback dials and extra reconnect delay.</comment>
<file context>
@@ -124,37 +124,84 @@ pub async fn connect_utp_plaintext(
+fn alternate_dial_cannot_help(err: &std::io::Error) -> bool {
+ let message = err.to_string();
+ message == "info hash mismatch" || message.starts_with("self-connection ")
+}
+
</file context>
| message == "info hash mismatch" || message.starts_with("self-connection ") | |
| message.starts_with("info hash mismatch") || message.starts_with("self-connection ") |
| for t in magnet.trackers.iter().chain(extra_trackers.iter()) { | ||
| for part in expand_tracker_entries(t) { | ||
| if !trackers.iter().any(|x| x == &part) { | ||
| trackers.push(part); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
P2: Magnet trackers with a literal comma in their URL can be corrupted before announce, because decoded tr= values are passed through expand_tracker_entries and split on ,. Consider only applying comma/newline expansion to caller-supplied tracker lists, while preserving each magnet tr parameter as one URL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/risuko-bt/src/magnet.rs, line 159:
<comment>Magnet trackers with a literal comma in their URL can be corrupted before announce, because decoded `tr=` values are passed through `expand_tracker_entries` and split on `,`. Consider only applying comma/newline expansion to caller-supplied tracker lists, while preserving each magnet `tr` parameter as one URL.</comment>
<file context>
@@ -34,73 +30,146 @@ const OUR_UT_PEX_ID: u8 = 4;
- if !trackers.iter().any(|x| x == t) {
- trackers.push(t.clone());
+ let mut trackers: Vec<String> = Vec::new();
+ for t in magnet.trackers.iter().chain(extra_trackers.iter()) {
+ for part in expand_tracker_entries(t) {
+ if !trackers.iter().any(|x| x == &part) {
</file context>
| for t in magnet.trackers.iter().chain(extra_trackers.iter()) { | |
| for part in expand_tracker_entries(t) { | |
| if !trackers.iter().any(|x| x == &part) { | |
| trackers.push(part); | |
| } | |
| } | |
| } | |
| for t in &magnet.trackers { | |
| if !trackers.iter().any(|x| x == t) { | |
| trackers.push(t.clone()); | |
| } | |
| } | |
| for t in extra_trackers { | |
| for part in expand_tracker_entries(t) { | |
| if !trackers.iter().any(|x| x == &part) { | |
| trackers.push(part); | |
| } | |
| } | |
| } |
| fn is_cloudflare_v4(ip: std::net::Ipv4Addr) -> bool { | ||
| let o = ip.octets(); | ||
| matches!( | ||
| (o[0], o[1]), | ||
| (162, 158 | 159) // 162.158.0.0/15 | ||
| | (172, 64..=71) // 172.64.0.0/13 | ||
| | (104, 16..=31) // 104.16.0.0/12 (covers /13 published ranges) | ||
| | (173, 245) // 173.245.48.0/20 — coarse; rare false positives OK | ||
| | (108, 162) // 108.162.192.0/18 | ||
| | (141, 101) // 141.101.64.0/18 | ||
| | (188, 114) // 188.114.96.0/20 | ||
| | (190, 93) // 190.93.240.0/20 | ||
| | (197, 234) // 197.234.240.0/22 | ||
| | (198, 41) // 198.41.128.0/17 | ||
| ) || matches!( | ||
| (o[0], o[1], o[2]), | ||
| (103, 21, 244..=247) | (103, 22, 200..=203) | (103, 31, 4..=7) | (131, 0, 72..=75) | ||
| ) | ||
| } |
There was a problem hiding this comment.
P2: Peer discovery can drop legitimate peers in non-Cloudflare parts of these address blocks, because several CIDR checks only compare the first two octets despite narrower ranges. Narrow the checks to the actual prefix before using this as a global dial filter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/risuko-bt/src/magnet.rs, line 457:
<comment>Peer discovery can drop legitimate peers in non-Cloudflare parts of these address blocks, because several CIDR checks only compare the first two octets despite narrower ranges. Narrow the checks to the actual prefix before using this as a global dial filter.</comment>
<file context>
@@ -306,6 +414,66 @@ pub async fn resolve_with_peers(
+ }
+}
+
+fn is_cloudflare_v4(ip: std::net::Ipv4Addr) -> bool {
+ let o = ip.octets();
+ matches!(
</file context>
| fn is_cloudflare_v4(ip: std::net::Ipv4Addr) -> bool { | |
| let o = ip.octets(); | |
| matches!( | |
| (o[0], o[1]), | |
| (162, 158 | 159) // 162.158.0.0/15 | |
| | (172, 64..=71) // 172.64.0.0/13 | |
| | (104, 16..=31) // 104.16.0.0/12 (covers /13 published ranges) | |
| | (173, 245) // 173.245.48.0/20 — coarse; rare false positives OK | |
| | (108, 162) // 108.162.192.0/18 | |
| | (141, 101) // 141.101.64.0/18 | |
| | (188, 114) // 188.114.96.0/20 | |
| | (190, 93) // 190.93.240.0/20 | |
| | (197, 234) // 197.234.240.0/22 | |
| | (198, 41) // 198.41.128.0/17 | |
| ) || matches!( | |
| (o[0], o[1], o[2]), | |
| (103, 21, 244..=247) | (103, 22, 200..=203) | (103, 31, 4..=7) | (131, 0, 72..=75) | |
| ) | |
| } | |
| fn is_cloudflare_v4(ip: std::net::Ipv4Addr) -> bool { | |
| let [a, b, c, _] = ip.octets(); | |
| matches!( | |
| (a, b), | |
| (162, 158 | 159) // 162.158.0.0/15 | |
| | (172, 64..=71) // 172.64.0.0/13 | |
| | (104, 16..=31) // 104.16.0.0/12 | |
| ) || matches!( | |
| (a, b, c), | |
| (173, 245, 48..=63) // 173.245.48.0/20 | |
| | (108, 162, 192..=255) // 108.162.192.0/18 | |
| | (141, 101, 64..=127) // 141.101.64.0/18 | |
| | (188, 114, 96..=111) // 188.114.96.0/20 | |
| | (190, 93, 240..=255) // 190.93.240.0/20 | |
| | (197, 234, 240..=243) // 197.234.240.0/22 | |
| | (198, 41, 128..=255) // 198.41.128.0/17 | |
| | (103, 21, 244..=247) | |
| | (103, 22, 200..=203) | |
| | (103, 31, 4..=7) | |
| | (131, 0, 72..=75) | |
| ) | |
| } |
| return "local disconnect".into(); | ||
| } |
There was a problem hiding this comment.
P2: The peer actor now tears down the socket without an explicit shutdown(), so a local disconnect or writer exit may not be observed promptly by the remote peer and any queued bytes rely on task drop instead of an orderly close.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/risuko-bt/src/peer/connection.rs, line 1278:
<comment>The peer actor now tears down the socket without an explicit `shutdown()`, so a local disconnect or writer exit may not be observed promptly by the remote peer and any queued bytes rely on task drop instead of an orderly close.</comment>
<file context>
@@ -1234,15 +1269,16 @@ async fn writer_task(
if disconnect {
- let _ = writer.shutdown().await;
- return;
+ return "local disconnect".into();
}
}
</file context>
| return "local disconnect".into(); | |
| } | |
| let _ = writer.shutdown().await; | |
| return "local disconnect".into(); |
Summary by cubic
Fixes slow torrent speeds and resolves the Stats speed chart hover jitter. Improves peer connectivity, request scheduling, and parsing safety for faster, more reliable downloads.
Bug Fixes
Refactors
risuko-bt: Added an adaptive per‑torrent request budget with per‑peer caps; defaultbt-max-outstanding-per-peeris now 0 (auto 6–96).connect_prefer_utppath improve reachability behind NATs/filters.risuko-engine.Written for commit 5344e85. Summary will update on new commits.